Skip to content

Conversation

shwstppr
Copy link
Contributor

@shwstppr shwstppr commented Jul 29, 2025

Description

Fixes #9505

Trim the generated name to take the last 15 characters.

Types of changes

  • Breaking change (fix or feature that would cause existing functionality to change)
  • New feature (non-breaking change which adds functionality)
  • Bug fix (non-breaking change which fixes an issue)
  • Enhancement (improves an existing feature and functionality)
  • Cleanup (Code refactoring and cleanup, that may add test cases)
  • build/CI
  • test (unit or integration test code)

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

  • Major
  • Minor

Bug Severity

  • BLOCKER
  • Critical
  • Major
  • Minor
  • Trivial

Screenshots (if appropriate):

How Has This Been Tested?

How did you try to break this feature and the system with this change?

Copy link

codecov bot commented Jul 29, 2025

Codecov Report

❌ Patch coverage is 66.66667% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 17.36%. Comparing base (7d59bfe) to head (16240b9).
⚠️ Report is 27 commits behind head on main.

Files with missing lines Patch % Lines
...ava/com/cloud/network/as/AutoScaleManagerImpl.java 66.66% 4 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff            @@
##               main   #11327   +/-   ##
=========================================
  Coverage     17.35%   17.36%           
- Complexity    15232    15237    +5     
=========================================
  Files          5885     5886    +1     
  Lines        525618   525694   +76     
  Branches      64157    64163    +6     
=========================================
+ Hits          91225    91269   +44     
- Misses       424094   424124   +30     
- Partials      10299    10301    +2     
Flag Coverage Δ
uitests 3.63% <ø> (ø)
unittests 18.40% <66.66%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

shwstppr added 2 commits July 30, 2025 11:11
Signed-off-by: Abhishek Kumar <[email protected]>
Signed-off-by: Abhishek Kumar <[email protected]>
@shwstppr
Copy link
Contributor Author

@blueorangutan package

@blueorangutan
Copy link

@shwstppr a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

@blueorangutan
Copy link

Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 14463

Copy link

This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch.

@weizhouapache
Copy link
Member

@shwstppr
is this ready for review ?

@shwstppr
Copy link
Contributor Author

@weizhouapache yes, though I wanted to confirm if it is a good idea to use the last 15 characters of the long generated name. Not using the first 15 as they can turn out to be the same.

@shwstppr shwstppr marked this pull request as ready for review August 25, 2025 11:02
@weizhouapache
Copy link
Member

@weizhouapache yes, though I wanted to confirm if it is a good idea to use the last 15 characters of the long generated name. Not using the first 15 as they can turn out to be the same.

@shwstppr
looks ok to me. I could not find a better solution to be honest.

is it possible the first letter is unaccepted (for example -) after the truncation ?

Signed-off-by: Abhishek Kumar <[email protected]>
if (Character.isLetterOrDigit(hostName.charAt(0))) {
return new Pair<>(hostName, name);
}
String temp = name.substring(0, Math.max(0, name.length() - 15)).replaceAll("[^a-zA-Z0-9]", "");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

edge case: all of the last 15 chars are special chars or whitespace. I think we better do a replace first and then take the last 15.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@DaanHoogland I don't think this case can happen.
The only user-defined part in the VM name is the name of the autoscale group, and we've checks for that to only contain letters, numbers and hyphen. checkAutoScaleVmGroupName` does that

Signed-off-by: Abhishek Kumar <[email protected]>
@shwstppr
Copy link
Contributor Author

@blueorangutan package

@blueorangutan
Copy link

@shwstppr a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

@blueorangutan
Copy link

Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 14730

}
return new Pair<>(hostName, displayName);
String hostName = name.substring(Math.max(0, name.length() - 15));
if (Character.isLetterOrDigit(hostName.charAt(0))) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it seems digit it not supported as first char ? @shwstppr

public void checkNameForRFCCompliance(String name) {
if (!NetUtils.verifyDomainNameLabel(name, true)) {
throw new InvalidParameterValueException("Invalid name. Vm name can contain ASCII letters 'a' through 'z', the digits '0' through '9', "
+ "and the hyphen ('-'), must be between 1 and 63 characters long, and can't start or end with \"-\" and can't start with digit");
}
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

Autoscale VM Name Issue with Windows OS
5 participants